fix: remediate critical security vulnerabilities (CSRF, negative amounts, race conditions)#176
Open
devin-ai-integration[bot] wants to merge 4 commits intoDevOpsfrom
Open
Conversation
…nts, race conditions) - Enable CSRF protection in SecurityConfig (was globally disabled) - Add positive amount validation on deposit, withdraw, transfer operations - Add @transactional and pessimistic locking to prevent race conditions - Add username/password input validation on registration - Update Thymeleaf templates to use th:action for CSRF token injection - Add error handling for deposit controller endpoint - Add H2 test profile for local security testing - Add min=1 constraint on HTML amount input fields Co-Authored-By: Angela Lin <angela.lin@cognition.ai>
Devin Review
|
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: Angela Lin <angela.lin@cognition.ai>
…dynamic error messages - Fix ABBA deadlock risk in transferAmount by acquiring locks in alphabetical username order - Fix register.html to display dynamic error messages instead of hardcoded 'User already present' Co-Authored-By: Angela Lin <angela.lin@cognition.ai>
Co-Authored-By: Angela Lin <angela.lin@cognition.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Remediates 5 confirmed security vulnerabilities identified by the Hunter Agent Pipeline security audit:
Critical — Dynamically Confirmed Exploitable:
csrf.disable()). Updated all Thymeleaf forms to useth:actionfor automatic CSRF token injection.POST /withdraw amount=-5000created $5,000 from nothing.Critical — Code-Confirmed:
3. FINOPS-RACE-001: Concurrent Withdraw Double-Spend — Added
@Transactional+PESSIMISTIC_WRITElocking on all financial operations.4. FINOPS-RACE-002: Concurrent Transfer Double-Spend — Same fix pattern applied to transfer operations, with deterministic lock ordering (alphabetical by username) to prevent ABBA deadlocks.
Additional Hardening:
5. Input validation on registration (username format, password length, enumeration mitigation)
6. Error handling added to deposit controller endpoint
7. HTML
min="1"constraint on amount input fields8. H2 in-memory database profile added for local security testing
9. Register template updated to show dynamic error messages instead of hardcoded text
Files Changed (9)
SecurityConfig.java— CSRF re-enabledAccountService.java— Amount validation, @transactional, pessimistic locking with deterministic ordering, input validationBankController.java— Deposit error handlingAccountRepository.java— Pessimistic locking querydashboard.html,login.html,register.html— th:action for CSRF tokens, min amount, dynamic errorsapplication-h2.properties— H2 test profilepom.xml— H2 dependency for testingReview & Testing Checklist for Human
POST /deposit amount=-100andPOST /withdraw amount=-500— both should show error, balance unchangedAccountService.java— all financial methods usefindByUsernameForUpdate()with@Lock(PESSIMISTIC_WRITE), transfer uses deterministic lock orderingRecommended test plan: Start the app with H2 profile (
./mvnw spring-boot:run -Dspring-boot.run.profiles=h2), register two accounts, deposit funds, then verify all the above checklist items.Notes
application-h2.properties) is for local testing only and does not affect the production MySQL configuration.transferAmount()and dynamic error messages inregister.html.Link to Devin session: https://app.devin.ai/sessions/24fd567d7b324669bcf46b04821b01a0
Requested by: @angelalincog